Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-44488: Add tests for the --raise-on-partial-outputs option to pipetask. #25

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

TallJimbo
Copy link
Member

@TallJimbo TallJimbo commented Aug 23, 2024

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes (in ctrl_mpexec)

Copy link

@parejkoj parejkoj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the most concerning comment of the ones I've left: we specifically added error info to the task metadata, so I'm surprised it's not getting output.

data/SConscript Outdated
Comment on lines 93 to 98
# Configure one quantum to fail with AnnotatedPartialOutputsError.
# By default this is treated as a qualified success, and downstream
# quanta will be run (we expect them to them do NoWorkFound, which
# just reflects the fact that it's a little weird to pick ISR as
# the task to raise this error, but that doesn't matter for testing
# the mechanics.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's some useful clarification.

Comment on lines +189 to +192
# When we do raise, the ISR quantum that raised should not have
# metadata written, but it should have logs, and downstream tasks
# should not have either, because they are never run.
self.assertFalse(helper.butler.exists(get_mock_name("isr_metadata"), data_id, exposure=95104))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned that the metadata isn't written on a failure: we add information about the failure to the task metadata, so I'd have hoped that it would get written.

Copy link
Member Author

@TallJimbo TallJimbo Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately we'll break a lot of things if we start writing metadata for tasks that fail; I'd like to fix that eventually, but it's a couple of big projects down from the top of my work stack (I consider it part of the "butler provenance" work package). There may be a somewhat nearer-term possibility of getting it into the "quantum execution reports" that I believe BPS uses, but I'm not super familiar with how information flows into and out of those.

To be clear, the metadata absolutely is being written when we take the "qualified success" route.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, the task metadata on qualified success is probably good enough; AP's use case doesn't care about the outputs of failing tasks, so it shouldn't matter.

@TallJimbo TallJimbo merged commit afc067c into main Aug 28, 2024
6 checks passed
@TallJimbo TallJimbo deleted the tickets/DM-44488 branch August 28, 2024 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants